feat(polish): expanded feedback build_info + SC sweep + lexical warm-up#14
Merged
Conversation
Three things in this slice:
1. The header feedback icon now templates the FULL build_info block
(both deploy + data) into the GitHub-issue body, not just the
short SHA. Reporters get: page URL, user-agent, deploy
code_revision (full + short) + built_at, data code_revision +
corpus_state_key + stage4_rollup_state_key + built_at. Recomputes
on route change via `$page.url.href`.
2. T096 polish: new `site/src/tests/e2e/sc-sweep.spec.ts` running
the locally-observable SC items against production
(TARGET_BASE-configurable). Coverage:
SC-002 search latency (warm-path; waits for semantic worker
to settle, then measures keystroke →
result-count update)
SC-003 cell-switch (model dropdown change → UMAP title swap)
SC-004 mobile overflow (360 × 640 viewport, no horizontal scroll)
SC-005 accepted-only (window.__abstracts has 0 "Withdrawn" rows)
SC-011 footer SHA (consistent across home / about /
permalink routes)
3. Pre-warm the lexical inverted index during onMount via
requestIdleCallback (or a 200 ms setTimeout fallback). The index
was built lazily on first search, which made SC-002 measure a
~1.2 s cold build instead of the actual user-typing latency.
Pre-warming on idle moves that cost off the critical paint path
and lets the search keystroke land on a populated WeakMap.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request enhances the feedback reporting system by including detailed build and data environment information in the GitHub issue template and optimizes initial page performance by pre-warming the lexical search index during idle periods. Additionally, a new end-to-end test suite was added to verify success criteria such as search latency and mobile responsiveness. Reviewer feedback suggests improving test reliability and maintainability by adopting Playwright's standard fixtures, avoiding the suppression of timeout errors, and replacing hardcoded delays with state-based assertions.
Comment on lines
+39
to
+48
| test('SC-002 — search latency: typing returns a filtered count in < 500 ms (warm path)', async () => { | ||
| // In a real session the semantic worker pre-warms during page load, | ||
| // so by the time the user types, it's ready. We mirror that here: | ||
| // wait for the ✨ Semantic toggle to drop its `loading` class | ||
| // (i.e., the worker reached `ready`) before measuring keystroke | ||
| // latency. Cold-start latency is bounded by the MiniLM ONNX | ||
| // download (~23 MB) and is reported separately below for context. | ||
| const browser = await chromium.launch(); | ||
| const ctx = await browser.newContext({ viewport: { width: 1440, height: 900 } }); | ||
| const page = await ctx.newPage(); |
There was a problem hiding this comment.
Using the page fixture is the standard and recommended way to write Playwright tests. It automatically handles browser and context creation/cleanup, and ensures the test respects the global configuration (like baseURL or viewport) defined in playwright.config.ts. Manually launching the browser with chromium.launch() is less efficient and requires manual cleanup.
Note: This pattern repeats in all tests in this file; consider updating all of them and removing the manual browser.close() calls.
Suggested change
| test('SC-002 — search latency: typing returns a filtered count in < 500 ms (warm path)', async () => { | |
| // In a real session the semantic worker pre-warms during page load, | |
| // so by the time the user types, it's ready. We mirror that here: | |
| // wait for the ✨ Semantic toggle to drop its `loading` class | |
| // (i.e., the worker reached `ready`) before measuring keystroke | |
| // latency. Cold-start latency is bounded by the MiniLM ONNX | |
| // download (~23 MB) and is reported separately below for context. | |
| const browser = await chromium.launch(); | |
| const ctx = await browser.newContext({ viewport: { width: 1440, height: 900 } }); | |
| const page = await ctx.newPage(); | |
| test('SC-002 — search latency: typing returns a filtered count in < 500 ms (warm path)', async ({ page }) => { | |
| // In a real session the semantic worker pre-warms during page load, | |
| // so by the time the user types, it's ready. We mirror that here: | |
| // wait for the ✨ Semantic toggle to drop its `loading` class | |
| // (i.e., the worker reached `ready`) before measuring keystroke | |
| // latency. Cold-start latency is bounded by the MiniLM ONNX | |
| // download (~23 MB) and is reported separately below for context. | |
| await page.goto(`${BASE}/`, { waitUntil: 'load' }); |
| }, | ||
| { timeout: 30000 } | ||
| ) | ||
| .catch(() => null); |
There was a problem hiding this comment.
Swallowing the timeout error from waitForFunction with .catch(() => null) can lead to non-deterministic test results. If the semantic worker fails to settle within the timeout, the test should ideally fail with a clear error message rather than continuing with potentially stale or incorrect data.
);| await page.goto(`${BASE}/`, { waitUntil: 'load' }); | ||
| await page.waitForSelector('[data-testid="umap-chart-2d"]', { timeout: 30000 }); | ||
| // Wait for initial UMAP render | ||
| await page.waitForTimeout(2000); |
There was a problem hiding this comment.
Hardcoded waits like page.waitForTimeout(2000) are a common source of test flakiness and slow down the test suite. It is better to wait for a specific UI state or element to appear. For example, you could wait for a specific SVG element or data-point within the UMAP chart to be rendered.
satra
added a commit
that referenced
this pull request
May 18, 2026
… typo-recall eval (#18) * chore(stage6): wrap-up — Playwright per-US specs, facets unit test, typo-recall eval Closes out the remaining Stage 6 todos that were deferred to "Phase 11 polish" pending a live preview / built data package. Now that the data package is local and production is up, they're all genuinely runnable. Tests added: - `site/src/tests/e2e/search.spec.ts` (T062) — FR-007 narrowing, FR-008 typo recovery on ≥ 7-char words, PR#17 operator grammar (`"phrase"` ≤ bare-AND set, `-word` subtracts, `OR` unions), and the ✨ semantic-only badge tooltip contract. - `site/src/tests/e2e/facets.spec.ts` (T068) — FR-005/FR-013: facet click narrows results, sibling-facet counts recompute against the narrowed set, `Clear` restores the corpus-wide count. - `site/src/tests/e2e/cart.spec.ts` (T071) — FR-006/SC-009: card-icon add + reload (localStorage persistence), `Clear` empties, `cart-email` produces a `mailto:` URL with the poster_id in the body. - `site/src/tests/e2e/tour.spec.ts` (T081) — US6 acceptance: CTA banner on first visit, "Start tour" opens `.shepherd-element`, dismissal hides the CTA while the header `Tour` button remains. - `site/src/tests/unit/facets.test.ts` (T064) — FR-013 nuance: a facet's own selections don't zero its sibling options' counts. 4 cases against a 4-abstract fixture. 75/75 unit tests pass. Eval script: - `scripts/eval_typo_recall.py` (T058a, SC-010) — ports the lexical- search core to Python and replays it against the data-package shards. Generates full-title probes (capped at 8 tokens) with one recoverable single-typo on a ≥ 4-char content word. Recall against the live shards: **0.9685 over 762 probes** (≥ 0.90 target). The threshold scheme (<4 → exact, 4–6 → DL ≤ 1, ≥ 7 → DL ≤ 2) confirmed sound. Exits 0 on pass / 3 on fail (so the script is usable as a CI gate later). Bookkeeping (T099 + T100 + several already-shipped tasks reconciled in `specs/008-ui-rewrite/tasks.md`): - T026 (US8 draft-PR verification) — DONE via PRs #9–#17 Deployments- box surface. - T090 (Lighthouse-CI) — DONE in PR #15. - T091 (a11y axe-core audit) — DONE in PRs #12 + #14. - T096 (SC sweep) — DONE; SC-002 / SC-003 / SC-004 / SC-005 / SC-011 measured against production via `sc-sweep.spec.ts`. - T097 (FR-021 / FR-022 acceptance) — DONE in spirit across PRs #9–#17. - T099 (reconciliation pass) — DONE in this commit. - T100 — marked OBSOLETE (Stage 6 shipped across 10+ PRs, not as one final consolidating PR). After this PR lands, the only Stage 6 work that remains is the operator-syntax UX shipped in PR #17 (post-spec enhancement) — the spec's 105 numbered tasks are complete. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(stage6-wrap): address gemini review — stable hash + web-first asserts Addresses 5 medium-priority gemini-code-assist comments on PR #18. scripts/eval_typo_recall.py — `hash(word) & 0xFFFFFFFF` was non- deterministic across Python processes (`PYTHONHASHSEED` randomisation), contradicting the docstring's "deterministic across runs" promise. Switched to `zlib.adler32(word.encode('utf-8'))` — a stable 32-bit hash that's plenty for seeding a per-word PRNG. Eval still passes: **recall@10 = 0.9894 over 378 probes** (was 0.9685 with the old randomised seed, well above the 0.90 target either way). site/src/tests/unit/facets.test.ts — replaced `JSON.stringify(...)` Map equality with Vitest's native `expect(...).toEqual(...)`. Walks the Map structurally and gives a useful diff on failure. site/src/tests/e2e/cart.spec.ts — converted `expect(await count())` one-shots to web-first `await expect(locator.first()).toBeVisible()` and `await expect(locator).toHaveCount(0)`. Dropped every `waitForTimeout` in this spec; Playwright auto-retries. site/src/tests/e2e/search.spec.ts — replaced the three `waitForTimeout` calls (250 ms between fills, 100 ms after clearing, 800 ms after a semantic query) with `expect.poll` based two-tick stability checks. The semantic-settle wait now has a 2 s budget and fails fast if the worker hangs, rather than always burning the hard-coded 800 ms. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Three small things bundled:
build_info(deploy + data SHAs, state-keys, built_at, user-agent, current URL) — so a maintainer can correlate every issue to the exact code + data revision that was live.site/src/tests/e2e/sc-sweep.spec.tsruns five locally-observable success criteria against the live production URL (TARGET_BASE-configurable). All five pass on the latest deploy: SC-002 search latency (warm), SC-003 cell-switch, SC-004 mobile overflow, SC-005 accepted-only, SC-011 footer SHA consistency.requestIdleCallbackafter the home page paints, so the user's first keystroke hits a populated WeakMap instead of paying ~1.2 s on cold build.